Skip to content

fix: clang-tidy lint step fails when no csrc files change - #6049

Open
andrewwhitecdw wants to merge 1 commit into
NVIDIA:mainfrom
andrewwhitecdw:bugfix/lint-clang-tidy-lint-step-fails-when-no
Open

fix: clang-tidy lint step fails when no csrc files change#6049
andrewwhitecdw wants to merge 1 commit into
NVIDIA:mainfrom
andrewwhitecdw:bugfix/lint-clang-tidy-lint-step-fails-when-no

Conversation

@andrewwhitecdw

@andrewwhitecdw andrewwhitecdw commented Aug 11, 2026 •
edited
Loading

Copy link
Copy Markdown

This PR addresses the following issue in .github/workflows/lint.yml: clang-tidy lint step fails when no csrc files change.

Changes

  • .github/workflows/lint.yml: split changed-file discovery from the grep filter so that git diff failures are no longer swallowed by the || true guard.

Details

--- a/.github/workflows/lint.yml
+++ b/.github/workflows/lint.yml
@@ -65,7 +65,8 @@ jobs:
           head_commit=$(git rev-parse FETCH_HEAD)
           # diff-filter for lower case letter:
           # https://github.com/git/git/commit/7f2ea5f0f2fb056314092cce23202096ca70f076
-          files=$(git --no-pager diff --diff-filter=d --name-only $head_commit | grep -e "csrc/.*\.cpp" -e "csrc/.*\.h" || true)
+          changed_files=$(git --no-pager diff --diff-filter=d --name-only "$head_commit")
+          files=$(printf '%s\n' "$changed_files" | grep -e "csrc/.*\.cpp" -e "csrc/.*\.h" || true)
           if [ -n "$files" ]; then
             echo "$files" | xargs -r lintrunner --take CLANGTIDY --force-color
           fi

The original single pipeline used || true after grep, which also masked failures from git diff (e.g., an unavailable base revision). By capturing git diff output first, a genuine file-discovery error now fails the step, while grep returning no matches is still treated as an empty file list and skips lintrunner safely.

Testing

Validated the discovery logic locally with a temporary shell script:

$ bash /tmp/test_lint_snippet.sh HEAD
no files to lint

$ bash /tmp/test_lint_snippet.sh 67d4d14db
would lint csrc/contiguity.cpp ...

$ bash -c 'changed_files=$(git diff ... deadbeef ...); ...'
fatal: ambiguous argument ...
invalid commit correctly failed

No new test file is added because the repository does not currently have a workflow-syntax test harness; the validation above exercises the exact commands used in the workflow.

@greptile-apps

greptile-apps Bot commented Aug 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR prevents the clang-tidy job from failing when no relevant C++ files changed while preserving failures from Git file discovery.

  • Captures the changed-file list before filtering it for csrc C++ sources and headers.
  • Invokes clang-tidy only when the filtered list is non-empty.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
.github/workflows/lint.yml Separates Git diff failures from the expected empty grep result and conditionally runs clang-tidy only for matching files.

Reviews (2): Last reviewed commit: "fix: clang-tidy lint step fails when no ..." | Re-trigger Greptile

greptile-apps[bot]
greptile-apps Bot reviewed Aug 11, 2026
View reviewed changes
Comment thread .github/workflows/lint.yml Outdated
@andrewwhitecdw
fix: clang-tidy lint step fails when no csrc files change
fb668e1 
Signed-off-by: andrewwhitecdw <andrewwhitecdw@users.noreply.github.com>
@andrewwhitecdw
andrewwhitecdw force-pushed the bugfix/lint-clang-tidy-lint-step-fails-when-no branch from 25a58fd to fb668e1 Compare August 17, 2026 22:22
@andrewwhitecdw

andrewwhitecdw commented Aug 17, 2026

Copy link
Copy Markdown
Author

@greptile-apps thanks for the review. I split the file-discovery pipeline into two steps: git diff output is captured first, and only the grep no-match case is guarded by || true. This keeps the empty-file-list skip intact while allowing genuine git diff failures (e.g., an unavailable base revision) to fail the step. The updated commit is fb668e1e0.

@greptile-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

The fix looks correct. The separation is clean:

  • git diff now runs unguarded, so a bad $head_commit or other git failure will propagate and fail the step as expected.
  • grep's exit code 1 (no matches) is still swallowed by || true, preserving the no-op behavior when no C++ files changed.
  • The -r flag on xargs is a good addition — it guards against lintrunner being invoked with an empty argument list if $files somehow ends up non-empty but whitespace-only.

One minor nit: printf '%s\n' "$changed_files" is correct here since echo can mangle strings starting with -e or containing backslashes, so that's the right call.

LGTM. Thanks for the clear explanation and the testing output.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@andrewwhitecdw